Fix tools tty detection (fix avrdude running in safe mode) #897

cmaglie · 2020-08-14T12:58:06Z

Some tools detects if the program is running from terminal by looking at the stdin/out bindings.
Previously stdin wasn't bound to any custom stream, this fact lead avrdude to think it was run from terminal (instead of a script) and start it in "safe-mode". This turn out to be a problem in some cases, see #844

From the avrdude source code:

  if (isatty(STDIN_FILENO) == 0)
      safemode  = 0;       /* Turn off safemode if this isn't a terminal */

So we need to ensure that isatty(STDIN_FILENO) returns 0 when we run tools.
I've tried to run the following test program:

#include <stdio.h>
#include <unistd.h>
#include <io.h>

int main(void) {
    printf("%d\n", isatty(STDIN_FILENO));
    return 0;
}

compiled with:

x86_64-pc-cygwin-gcc test.c -o test-cygwin

and

i686-w64-mingw32-gcc test.c -o test-mingw32

using the following launcher in go:

func main() {
	cmd, err := executils.Command([]string{"test-cygwin.exe"})
	if err != nil {
		log.Fatalf("cannot execute upload tool: %s", err)
	}

	var o, e bytes.Buffer
	outStream := bufio.NewWriter(&o)
	errStream := bufio.NewWriter(&e)
	inStream := bufio.NewReader(&o)
	cmd.Stdout = outStream
	cmd.Stderr = errStream
	cmd.Stdin = inStream      // <---
	if err := cmd.Start(); err != nil {
		log.Fatalf("cannot execute upload tool: %s", err)
	}

	err = cmd.Wait()
	fmt.Println(o.String())
	if err != nil {
		log.Fatalf("uploading error: %s", err)
	}
}

I tried with and without the cmd.Stdin = inStream:

the cygwin build is unaffected it always returns 0
the mingw32 build instead returns 64 without the stdin redirect and 0 with the stdin redirect.

This makes me think that avrdude has been built with mingw32 and this PR should fix the issue, but a direct test is required.

Fix: #844

They are immediatly overwritten on the next line.

This is required because some tools detects if the program is running from terminal by looking at the stdin/out bindings. Fix: arduino#844

matthijskooijman · 2020-08-14T13:48:32Z

If running with safemode is the problem here, wouldn't a cleaner fix be to run avrdude with -s to disable safemode explicitly? That would also need to happen in third-party cores, so that's a bit more tricky. OTOH, thinking on this again, since we're not sending any input to avrdude, closing its stdin or redirecting it to null would be fine.

Looking at the code, this adds custom readers and writers to create null streams, which seems rather inefficient to me (since it involves creating an extra pipe pair underwater, I think). A simpler way would be to just set .e.g Stdin to nil, which redirects to /dev/null (or the equivalent), see docs. Did you consider that option?

cmaglie · 2020-08-14T15:06:32Z

that would also need to happen in third-party cores

yes, that's the problem, adding -s won't fix the issue in general. This is worsened by the fact that this happens only with the arduino-cli and not with the Arduino IDE, so 3rd party maintainers are less encouraged to fix it.

Looking at the code, this adds custom readers and writers to create null streams, which seems rather inefficient to me (since it involves creating an extra pipe pair underwater, I think). A simpler way would be to just set .e.g Stdin to nil, which redirects to /dev/null (or the equivalent), see docs. Did you consider that option?

I tried that, but they are nil by default, setting it explicitly doesn't seem to affect the call to isatty.
I don't know which checks performs isatty under mingw32 but, if we want it to return 0, it seems that we need to create a completely different stream. BTW a pipe has a very little cost in term of efficiency, considering that we already use 2 to get the stdout and stderr.

matthijskooijman · 2020-08-14T15:56:31Z

I tried that, but they are nil by default, setting it explicitly doesn't seem to affect the call to isatty.

Hm, good point.

In the original report @per1234 mentions this problem occurs on Windows and Linux/Ubuntu, but you're only mentioning Windows here. I also tried reproducing it (with your testcode, not full arduino-cli) on Ubuntu, but there it seems to work as expected (istty returning false for /dev/null). Did anything change since that report already maybe?

Looking at the golang exec package, it seems that Stdin = nil means it opens os.DevNull:

https://github.com/golang/go/blob/d0d6593d1d4e81acd073244f42b6893fa65c99d8/src/os/exec/exec.go#L245-L252

which on windows is just a special file called NUL:

https://github.com/golang/go/blob/e64675a79fef5924f268425de021372df874010e/src/os/file_windows.go#L97

So that sounds like it should work. Maybe isatty does something funny on mingw32 with NUL somehow?

This post suggests that the "64 return value for NUL" is actually the case with _isatty, which is a Windows-specific function (docs) that determines whether an FD is a character device (rather than an actually tty, like Linux isatty), and I suppose NUL might be a character device. I also suppose that mingw might just substitute isatty() with _isatty() (also suggested here: https://sourceforge.net/p/mingw-w64/mailman/message/35498600/). It seems that this still happens with mingw git version (see file comment here), not sure about different mingw versions, though.

It seems that the ideal fix for this would maybe be in mingw, which would then be used to recompile a fixed avrdude. However, that's quite complicated, so maybe your solution is indeed the easiest way forward. I do wonder if this redirection needs to happen for stdout/stderr as well (seems to be overridden in practice), maybe just keep the hack as small as possible and only apply it for stdin?

One alternative could be to simply close stdin rather than redirect it to /dev/null, which I think would also make isatty return false. However, it seems that the golang exec module does not support this in any way (it would involve stdin() returning nil to pass a nil file descriptor to os.startProcess around this bit of code, but I don't think we can override this in such a way.

per1234 · 2020-08-14T17:16:00Z

In the original report @per1234 mentions this problem occurs on Windows and Linux/Ubuntu

Sorry, I think my report was unclear. When I said this:

If you run the identical upload command from the command line on Linux or Windows, you get the same failure shown above.

I meant running the avrdude command directly, not using arduino-cli.

This is not strictly required for the 'avrdude' hack.

per1234

I verified that I can still reproduce #844 with Arduino CLI @master, but that #844 no longer occurs when using Arduino CLI @ 82a957f.

Thanks @cmaglie!

cmaglie added 2 commits August 14, 2020 14:40

Removed useless stdout/err listeners

95585ac

They are immediatly overwritten on the next line.

Always pipe stdout/err/in when running tools.

fb3f701

This is required because some tools detects if the program is running from terminal by looking at the stdin/out bindings. Fix: arduino#844

cmaglie requested a review from per1234 August 14, 2020 12:58

cmaglie self-assigned this Aug 14, 2020

cmaglie added component/core labels Aug 14, 2020

cmaglie requested a review from ubidefeo August 14, 2020 12:58

Do not use NullWriter in executils by default.

82a957f

This is not strictly required for the 'avrdude' hack.

per1234 approved these changes Aug 18, 2020

View reviewed changes

cmaglie merged commit 1785314 into arduino:master Aug 18, 2020

cmaglie deleted the fix_avrdude_weirdness branch August 18, 2020 10:14

per1234 added os: windows Specific to Windows operating system topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jan 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tools tty detection (fix avrdude running in safe mode) #897

Fix tools tty detection (fix avrdude running in safe mode) #897

cmaglie commented Aug 14, 2020

matthijskooijman commented Aug 14, 2020

cmaglie commented Aug 14, 2020

matthijskooijman commented Aug 14, 2020

per1234 commented Aug 14, 2020

per1234 left a comment

Fix tools tty detection (fix avrdude running in safe mode) #897

Fix tools tty detection (fix avrdude running in safe mode) #897

Conversation

cmaglie commented Aug 14, 2020

matthijskooijman commented Aug 14, 2020

cmaglie commented Aug 14, 2020

matthijskooijman commented Aug 14, 2020

per1234 commented Aug 14, 2020

per1234 left a comment

Choose a reason for hiding this comment